Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: refactor test-dns-regress-6244.js #13058

Closed
wants to merge 1 commit into from
Closed

test: refactor test-dns-regress-6244.js #13058

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 16, 2017

  • Move from parallel to internet because it performs a DNS query
  • Provide link to relevant issue in comments
  • Remove unnecessary explicit no-op function
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test dns

@Trott
test: refactor test-dns-regress-6244.js
07aa6f7 
* Move from parallel to internet because it performs a DNS query
* Provide link to relevant issue in comments
* Remove unnecessary explicit no-op function
@Trott Trott added dns Issues and PRs related to the dns subsystem. test Issues and PRs related to the tests. labels May 16, 2017
@Trott
Copy link
Member Author

Trott commented May 16, 2017

@nodejs/testing

@gibfahn
Copy link
Member

gibfahn commented May 16, 2017

My only issue with this would be that the internet tests don't get run very much, so moving to internet basically means ignoring. Of course if there's no feasible way to do this offline then we have no choice.

@Trott
Copy link
Member Author

Trott commented May 16, 2017

My only issue with this would be that the internet tests don't get run very much, so moving to internet basically means ignoring.

@gibfahn Discussion to come up with a solution to that at #13061

@addaleax
Copy link
Member

Landed in 47e3d00

@addaleax addaleax closed this May 18, 2017
Trott added a commit to Trott/io.js that referenced this pull request May 18, 2017
@Trott @addaleax
test: refactor test-dns-regress-6244.js
47e3d00 
* Move from parallel to internet because it performs a DNS query
* Provide link to relevant issue in comments
* Remove unnecessary explicit no-op function

PR-URL: nodejs#13058
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
@Trott
test: refactor test-dns-regress-6244.js
fccd34a 
* Move from parallel to internet because it performs a DNS query
* Provide link to relevant issue in comments
* Remove unnecessary explicit no-op function

PR-URL: nodejs#13058
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

This test does not pass on v6.x

Please feel free to backport

@Trott
Copy link
Member Author

Trott commented Jun 23, 2017

This test does not pass on v6.x

@MylesBorins Are you sure? That's strange because this moves the test from parallel to internet which means make test and make test-ci now no longer run the test. The only thing this would need to pass is linting.

@gibfahn
Copy link
Member

gibfahn commented Jun 23, 2017

@Trott when backporting we normally run any tests that were changed, not just the ones that make test tests.

This fails as follows:

➜  node git:(v6.x-staging) ✗ ❯ tools/test.py test/internet/test-dns-regress-6244.js                                                                                                   ~/wrk/com/DANGER/node
=== release test-dns-regress-6244 ===
Path: internet/test-dns-regress-6244
/Users/gib/wrk/com/DANGER/node/test/common/index.js:431
    name: fn.name || '<anonymous>'
            ^

TypeError: Cannot read property 'name' of undefined
    at Object.exports.mustCall (/Users/gib/wrk/com/DANGER/node/test/common/index.js:431:13)
    at Object.<anonymous> (/Users/gib/wrk/com/DANGER/node/test/internet/test-dns-regress-6244.js:6:34)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:389:7)
    at startup (bootstrap_node.js:149:9)
Command: out/Release/node /Users/gib/wrk/com/DANGER/node/test/internet/test-dns-regress-6244.js
[00:00|% 100|+   0|-   1]: Done

Given that the only code change in the pr is common.mustCall( () => {} ) -> common.mustCall(), I'm thinking v6.x must be missing the PR to allow mustCall to take no parameters.

@Trott Trott deleted the 6244-refactor branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@santigimeno santigimeno santigimeno approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
dns Issues and PRs related to the dns subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Trott @gibfahn @addaleax @MylesBorins @santigimeno @lpinca @cjihrig